Ensure certificates are getting into cert manager#10073
Ensure certificates are getting into cert manager#10073anhu wants to merge 2 commits intowolfSSL:masterfrom
Conversation
|
This replaces #8708 |
Before this fix, the certificates were not getting into the certificate manager. This makes sure they are going in.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 1 skipped
Posted findings
- [Critical] test_X509_STORE_InvalidCa_NoCallback removed from .c but still declared and referenced in .h — linker error —
tests/api/test_ossl_x509_str.h:34,56 - [Medium] Defense-in-depth assertion removed from test_X509_STORE_InvalidCa without explanation —
tests/api/test_ossl_x509_str.c:1076-1077 - [Medium] Return value of X509StorePushCertsToCM silently ignored —
src/ssl_api_cert.c:1545
Skipped findings
- [Critical] test_X509_STORE_InvalidCa_NoCallback removed from .c but still declared and referenced in .h — linker error
Review generated by Skoll via openclaw
|
|
||
| ExpectIntEQ(X509_STORE_CTX_init(ctx, str, cert, untrusted), 1); | ||
| ExpectIntEQ(X509_verify_cert(ctx), 1); | ||
| ExpectIntEQ(last_errcode, X509_V_ERR_INVALID_CA); |
There was a problem hiding this comment.
🟡 [Medium] Defense-in-depth assertion removed from test_X509_STORE_InvalidCa without explanation
💡 SUGGEST test
The PR removes the assertion ExpectIntEQ(X509_STORE_CTX_get_error(ctx), X509_V_ERR_INVALID_CA) and its accompanying comment: "Defense in depth: ctx->error must not be clobbered back to X509_V_OK by the later successful verification of the intermediate against the trusted root. The worst-seen error must persist." This assertion was specifically guarding against error-clobbering regressions. Its removal, along with the complete removal of test_X509_STORE_InvalidCa_NoCallback, reduces test coverage for INVALID_CA error handling. If these were removed because the PR's changes to cert propagation altered the verification behavior (e.g., the error no longer persists), that behavioral change should be documented and potentially re-tested differently.
Suggestion:
| ExpectIntEQ(last_errcode, X509_V_ERR_INVALID_CA); | |
| If the behavior change is intentional (i.e., the error is no longer expected to persist because the cert now goes through a different path), add a comment explaining why. If it's not intentional, restore the assertion. Consider adding an equivalent test to verify the new behavior. |
| * X509_STORE_add_cert only go into store->certs, not the | ||
| * CertManager. Push them into the CM now so that all | ||
| * verification paths can find them. */ | ||
| X509StorePushCertsToCM(str); |
There was a problem hiding this comment.
🟡 [Medium] Return value of X509StorePushCertsToCM silently ignored
💡 SUGGEST convention
X509StorePushCertsToCM(str) returns WOLFSSL_FATAL_ERROR if any certificate fails to be added to the CertManager, but the return value is completely discarded. While wolfSSL_CTX_set_cert_store is a void function (matching the OpenSSL API), silently ignoring failures could mask problems — the caller would believe all certificates were pushed into the CM when some may have been silently dropped. At minimum, a WOLFSSL_MSG warning on failure would help with debugging.
Suggestion:
| X509StorePushCertsToCM(str); | |
| ```suggestion | |
| #ifdef OPENSSL_EXTRA | |
| /* Non-self-signed certs (intermediates) added via | |
| * X509_STORE_add_cert only go into store->certs, not the | |
| * CertManager. Push them into the CM now so that all | |
| * verification paths can find them. */ | |
| if (X509StorePushCertsToCM(str) != WOLFSSL_SUCCESS) { | |
| WOLFSSL_MSG("wolfSSL_CTX_set_cert_store: failed to push some " | |
| "certs to CertManager"); | |
| } | |
| #endif |
2 participants
Fixes ZD 19760